Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reformat docs; minor fixes #244

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Reformat docs; minor fixes #244

wants to merge 3 commits into from

Conversation

ggleyzer
Copy link
Collaborator

While using JsonPointer API I noticed that some of the docs are formatted with a style different from other classes in this package. This is a trivial change - put there as a draft for JK to review two suspicious places in the code

@ggleyzer ggleyzer requested a review from thegridman October 13, 2024 16:17
@@ -51,7 +50,7 @@ class JsonMergePatch(Doc patch) {
if (Doc targetValue := target.get(key)) {
merge(key, merge(targetValue, value, inPlace));
} else {
merge(key, merge(json.newObject(), value, True));
merge(key, merge(json.newObject(), value, True)); // TODO JK: why not "inPlace"
Copy link
Contributor

@thegridman thegridman Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are merging value into a new JsonObject so we may as well do it in place (hence the True argument). There is no point doing a copy if the inPlace parameter is False.

Also, I think line 31 is wrong, but I cannot add a comment to it on that line. I suspect it should be return merge(target, patch, inPlace);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... looking closer into the
private Doc merge(Doc doc, Doc patch, Boolean inPlace = False)
implementation I realized, that inPlace argument is not used for anything but a recursive call to itself. The actual value affects absolutely nothing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it appears to always be doing an in-place merge, i.e modifying the target directly. Maybe that needs fixing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not just that; the "inPlace" flag is not used for anything (e.g. ListMap construction)

*
* @param p the `Primitive` to merge
* @param obj the `Primitive` value to merge (TODO JK: value is not used!?)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... but if we remove it, then what is the point of calling this method? Don't we lose the value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you can merge one json object into another, or one json array into another, but you cannot merge anything into a primitive. So if the target field is a primitive, it is basically just replaced with the value being merged in.

e.g. If I have

{"foo": 1234}

so "foo" is a primitive number field. The I want to merge the array [1, 2, 3, 4] into field "foo" I end up with

{"foo": [1, 2, 3, 4]}

If "foo" was an array

{"foo": [9, 8, 7]}

and I did the same merge then I believe I would get

{"foo": [9, 8, 7, 1, 2, 3, 4]}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, yes we lose the original value if it was a primitive

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... I got confused by the doc then. The method doc says:

Deeply merge a `Primitive` value into this builder. 
...
@param doc   the value to merge the primitive into

So I thought the current value is supposed to be replaced by the primitive. I think it needs to be improved

@ggleyzer ggleyzer force-pushed the gene/doc branch 4 times, most recently from 0675723 to 2a45369 Compare October 22, 2024 16:43
@thegridman
Copy link
Contributor

I have submitted some updates.

  • I cleaned up the method docs in JsonBuilder.x as I think they were incorrect and certainly not clear.
  • I have refactored the merge code in JsonMergePatch.x to work as described in the RFC. I added some tests using examples in the RFC.
  • There are two TODO JK comments in the merge method in JsonMergePatch.x that need resolving

@ggleyzer
Copy link
Collaborator Author

Looks good!

@ggleyzer ggleyzer force-pushed the gene/doc branch 4 times, most recently from a860300 to 2685fc6 Compare October 29, 2024 20:55
@ggleyzer ggleyzer force-pushed the gene/doc branch 5 times, most recently from 9e7b0e3 to 9bd5344 Compare November 7, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants